fix: report incomplete batch gap-fill analysis - #797

[SkillSpector Review]

Hi @yashrajp22, thank you for routing the batch scanner's gap-fill pass through the core batch executor! A failed batch is now retried under the core policy, recorded in the inspection ledger and kept out of gap_fill_applied. Completeness is recomputed with finalize_ledger rather than by editing counters by hand, and the new integration test drives the real retry loop, ledger and report formatters.

Value and readiness: The problem is real on main. I ran main's own run_gap_fill and run_one with malformed JSON, {"findings": 1} and a provider RuntimeError. In each case run_gap_fill returned [] after one call per file with no retry, and run_one returned error=None with gap_fill_applied=True, is_complete=True, execution_successful=True and recommendation SAFE. I then transcribed the PR's gap_fill.py:218-306 and runner.py:778-802 (nothing was imported from the PR) and ran them on main's core, with and without deepseek_compat. Every failure inside a batch is now handled as described:

• Malformed, schema-invalid and BOM-prefixed JSON, and plain-text refusals, get 4 attempts and are then recorded as llm_structured_response_invalid.
• Provider errors are recorded as llm_batch_failed.
• In every failure mode, gap_fill_applied=False, is_complete=False, SAFE becomes CAUTION, an error is returned and the command exits 2.
• Core findings and findings from successful batches are kept.
• Valid empty responses still succeed after one call.

Two gaps remain against the PR's stated outcome ("Stops failed gap-fill analysis from being marked as successfully applied. Preserves existing findings and reports the failure."), and both block merge:

• A gap-fill failure before batching, such as the pool-mode TypeError, now replaces the entry with an ERROR stub and drops the core findings that main kept (finding 1).
• A JSON object without a findings key, such as {"error": ...} or {}, is still accepted as a successful, complete pass (finding 2).

The other findings are non-blocking. Once findings 1 and 2 are fixed and CI has run, the PR is ready for final maintainer review. The merge also needs sequencing with #796.

Material findings

1. [Blocker] contrib/batch_scan/runner.py:804: a gap-fill failure before batching now replaces the whole entry with an ERROR stub, which drops the core findings.
   • The broad except Exception: return [] is gone, so GapFillAnalyzer(...) and get_batches (gap_fill.py:301-302) now raise freely. run_one catches only GapFillError (runner.py:782), so any other exception reaches runner.py:804-805. entry_from_error (runner.py:811-838) then returns issues=[], score 0 and severity/recommendation ERROR.
   • Concrete trigger, which is pool mode:
     • create_api_key_pool_from_env returns a pool whenever at least 2 keys are configured, from SKILLSPECTOR_API_KEYS or from OPENAI_API_KEY plus OPENAI_API_KEY_2..9 (api_pool.py:603-664).
     • batch_scan.py:195-197 then calls set_api_pool, which installs _pooled_get_chat_model(model=None) (runner.py:89-98).
     • LLMAnalyzerBase.__init__ always calls get_chat_model(model=model, timeout=...) (llm_analyzer_base.py:938), so constructing GapFillAnalyzer raises TypeError: ... unexpected keyword argument 'timeout'.
     • This affects every non-English skill with use_llm and a non-empty provider-eligible cache.
   • I reproduced this with main's code, a stub pool and deepseek_compat, on a Chinese skill using the real graph:
     • main's run_one kept score 68, HIGH, DO NOT INSTALL and issues E1/PE3/SC2/TM2. It also reported gap_fill_applied=True, which was the old silent bug.
     • The transcribed PR run_one returned the TypeError text with {score 0, severity ERROR, recommendation ERROR} and issues: [].
     • The graph itself completes in pool mode. Core LLM analyzers already fail on main but are caught, so main's entry was already is_complete=False. The core findings are what this PR loses.
   • Scope: ValueError was already re-raised on main, so a missing-credential failure was an ERROR stub before this PR as well. The types whose behaviour changed are TypeError and other construction or batching errors, and NotImplementedError from run_batches_detailed.
   • Consequence: this fails closed (ERROR, exit 2), so it is not a bypass. But it contradicts "Preserves existing findings and reports the failure" for failures outside run_batches_detailed. The new test mocks get_chat_model with lambda **kw, so the test does not exercise this path.
   • Expected fix:
     • Treat gap-fill exceptions separately from graph exceptions. The simplest way is for run_gap_fill to wrap construction and get_batches failures in a GapFillError whose outcome marks every planned file as failed. The existing branch at runner.py:790-802 then keeps the core entry, marks it incomplete and returns the error.
     • Decide whether ValueError (configuration) should stay a hard error.
     • Add a test in which GapFillAnalyzer construction raises (for example, a get_chat_model that rejects timeout), and assert that TM1 survives.
     • The pool-mode timeout mismatch in _pooled_get_chat_model and compat Patch 1 is a separate bug on main and belongs in its own change.
2. [Blocker] contrib/batch_scan/gap_fill.py:260: a JSON object without a findings key is still accepted as a successful, complete pass.
   • GapFillResult.findings defaults to an empty list (gap_fill.py:94), so GapFillResult.model_validate(data) accepts any dict that lacks the key. run_gap_fill then raises nothing, and runner.py:788 sets gap_fill_applied=True. The PR pins this behaviour in contrib/batch_scan/tests/tests-pro/test_gap_fill.py:226-227.
   • Transcription on main's core: {"error": "I cannot help with that"}, {"refusal": "..."}, {} and a fenced {"error": ...} each gave 1 call, error=None, gap_fill_applied=True and 0 findings. A plain-text refusal and {"findings": null} correctly gave 4 calls and llm_structured_response_invalid.
   • This is not a regression. main returned [] for these inputs as well, and core's LLMAnalysisResult uses the same default (llm_analyzer_base.py:644). The gap is narrow: gap-fill runs in raw chat mode, so a provider refusal usually arrives as prose, and that case is caught.
   • Consequence: a non-compliant response that happens to be valid JSON is still reported as "applied, no findings, complete". If the core scan said SAFE, the skill stays SAFE. That is the silent pass this PR sets out to remove.
   • Expected fix:
     • In _parse_json_response, raise _StructuredResponseValidationError when not isinstance(data, dict) or "findings" not in data, or validate against a private model in which findings is required. Leave GapFillResult's default unchanged.
     • Change test_missing_findings_key_keeps_schema_default to expect the raise, and add {"other": "value"} and {} to the invalid list.
     • Trade-off: a bare {} meaning "nothing found" would then retry and end incomplete. That is defensible because the prompt requires the key (gap_fill.py:147-160).
3. [Non-blocking] contrib/batch_scan/gap_fill.py:199: gap-fill has no deadline. The new structured retries add time inside the 90 s worker budget, and the runtime_limit path cannot occur in production.
   • super().__init__(base_prompt=prompt, model=resolved_model) passes no timeout, and compat Patch 1 _patched_base_init (runner.py:128-136) has no timeout parameter. So _require_time_remaining() returns None (llm_analyzer_base.py:986-991), LLMRuntimeLimitError is never raised, and RUNTIME_LIMIT is never recorded for gap-fill.
   • The test's timeout case raises LLMRuntimeLimitError("deadline") from the fake model (tests/test_batch_scan_security.py:188) and sets the retry delays to zero (:195). Both are synthetic.
   • Retry cost:
     • Each malformed batch now gets STRUCTURED_RESPONSE_MAX_ATTEMPTS=4 calls with 0.5/1.0/2.0 s backoff (llm_analyzer_base.py:79-87), where main made 1 call.
     • Batches run one after another, and a large file can span several batches.
     • Transcription with the real delays, 2 malformed files: at L=0.2 s, main took 0.41 s for 2 calls and the PR 8.68 s for 8 calls (7.08 s of it backoff). At L=1.0 s, it was 2.01 s against 15.07 s.
     • That matches N·(4L+3.5 s) against N·L. By arithmetic (not run), 2 batches at L=10 s is about 87 s against 20 s, before core-graph time.
   • The worker is killed at 90 s (batch_scan.py:204, :232-234), and the supervisor substitutes an ERROR stub with no issues.
   • The missing deadline predates this PR, since main already retried provider and rate-limit errors. What the PR adds is the malformed-output retry cost. Per-request HTTP timeouts and provider errors are correctly recorded as llm_batch_failed, with findings kept.
   • Consequence: a slow provider that keeps returning malformed output can push a skill past the worker kill, where it would otherwise be reported as incomplete with findings kept. Readers may also take the timeout test and the PR body's "runtime limit" wording as covering production timeouts, which they do not.
   • Expected fix:
     • Thread a monotonic deadline through run_one → run_gap_fill → GapFillAnalyzer, for example timeout=lambda: deadline - time.monotonic() - margin. A counterfactual transcription with a 3 s deadline at L=0.5 s stopped after 3 calls in 3.03 s and recorded runtime_limit for both batches.
     • This needs Patch 1 to forward timeout, which #796 adds.
     • If the fix is deferred, drop "runtime limit" from the PR body's list of handled failures, note the per-batch retry cost, and relabel the synthetic timeout test case.
4. [Non-blocking] contrib/batch_scan/runner.py:802: every gap-fill incompleteness exits 2, but the JSON shows no error for the non-fatal modes.
   • runner.py:802 returns str(gap_error) for every GapFillError. batch_scan.py:468-473 prints a per-skill ERROR, and :534-535 exits 2. The entry never gets an error key, and reports.py:118 counts Errors: only from r.get("error").
   • Transcription of the PR branch on a real main graph result, passed to main's reports._format_json/_format_terminal:
     • json and schema: execution_successful=True, no skills[].error, failed_executions=0, incomplete_skills=1, row (llm_structured_response_invalid, fatal=False), no Errors: line, exit 2.
     • timeout: the same, with (runtime_limit, fatal=False), exit 2.
     • provider: execution_successful=False, failed_executions=1, (llm_batch_failed, fatal=True), exit 2.
   • Comparison:
     • Core skillspector scan exits 2 on a result only when execution_successful is False (cli.py:858-859).
     • Inside batch_scan, main's run_one returns error=None for core LLM failures. I measured a malformed core output and a fatal core provider failure (failed_executions=1), and both exit 0.
     • #796 does not change run_one's error return.
   • The JSON is not completely silent: gap_fill_applied: false, incomplete_skills and ledger_exceptions are present. But gap_fill_applied: false is also set for an empty cache with no error.
   • Consequence: a CI job that gets exit 2 finds no per-skill error and failed_executions: 0 for two of the three modes, and the same reason code exits differently depending on whether gap-fill or a core analyzer produced it. This fails closed but is inconsistent.
   • Expected fix: pick one rule and document it.
     • Option (a): return an error only when completeness["execution_successful"] is False. To treat core and gap-fill fatal failures the same way, batch_scan.py would also count entry.get("execution_successful") is False, which is a gap on main outside this PR.
     • Option (b): keep exit 2 and add a machine-readable field such as enhancements.gap_fill_status/gap_fill_error with the reason codes.
     • Either way, update the exit-code table in contrib/batch_scan/docs/README.md:310-316 and coordinate with #796.
5. [Non-blocking] tests/test_batch_scan_security.py:133: the new test does not assert two of the behaviours the PR claims.
   • The parametrized test runs in CI and would fail on main in every case. With the test's mocks, main's run_one returns error=None and gap_fill_applied=True, so the assertion at :201 fails. The core claim is covered.
   • The assertions (:201-234) never read entry["risk_assessment"] or entry["execution_successful"], and batch_scan's exit code depends only on errors.
   • Mutation check (transcription): deleting runner.py:799-801, the execution_successful copy and the SAFE→CAUTION change, still passes every assertion in all 8 cases. The recommendation starts as SAFE in this test, so lines 800-801 run but their result is never checked.
   • There is no case where the failure happens before run_batches_detailed (finding 1), and the timeout case is synthetic (finding 3).
   • The edits to contrib/batch_scan/tests/tests-pro/test_gap_fill.py do not run in CI: pyproject.toml:117 sets testpaths = ["tests"] and Makefile:110 runs pytest ... tests/. That file pins a BOM-prefixed valid response as a failure (:218) and a missing findings key as a valid empty result (:226).
   • Expected fix:
     • Add assert entry["risk_assessment"]["recommendation"] == "CAUTION" and assert entry["execution_successful"] is (failure_mode != "provider").
     • Optionally assert failed_executions == int(failure_mode == "provider").
     • Add a construction-failure case.
     • Optionally move the parse-failure unit cases under tests/.
6. [Non-blocking] contrib/batch_scan/runner.py:759: docstrings and docs do not reflect the changed contracts.
   • runner.py:756-760 says that on failure entry is a stub error entry. For a gap-fill failure, runner.py:802 returns the full entry, recomputed and marked incomplete, together with an error.
   • run_gap_fill's docstring (gap_fill.py:276-297) still says it returns "A (possibly empty) list" and has no Raises section. It now raises GapFillError (:305), and construction errors propagate.
   • The section header at gap_fill.py:266 still says "Backward-compatible entry point".
   • The parse_response docstring (:219-224) does not mention the new raise.
   • run_gap_fill is public API (contrib/batch_scan/__init__.py:30, :67), but GapFillError, which a caller needs in order to read .outcome.successful, is not exported.
   • contrib/batch_scan/docs/README.md:205 says gap_fill_applied is true "if LLM gap-fill was used". On partial success it is now false while gap_fill_findings > 0.
   • contrib/batch_scan/tests/docs/TEST_GUIDE.md:122 still lists 9 TestParseResponseInvalidInput tests; there are now 2 (the file goes from 35 to 28 tests).
   • Expected fix:
     • Add Raises: GapFillError (with .outcome) to run_gap_fill, and update run_one's Returns section and the stale header.
     • Optionally export GapFillError.
     • Define gap_fill_applied in the README as "true only if every gap-fill batch completed".
     • Update the TEST_GUIDE row.
7. [Non-blocking] contrib/batch_scan/runner.py:784: minor duplication, and a branch reachable only from tests.
   • The comprehension at runner.py:784 repeats collect_findings (llm_analyzer_base.py:1512-1525). The runner has no analyzer instance to call it on.
   • The isinstance(response, GapFillResult) branch (gap_fill.py:225-226) cannot be reached in production. response_schema = None (:192), so _invoke_batch always passes a str. The branch exists only to keep TestParseResponsePydanticModel passing; that test's len >= 0 assertion predates this PR.
   • Expected fix (optional): expose the retained findings on GapFillError (for example a findings attribute) and use it at runner.py:784. Either tighten the Pydantic-path test to assert the returned rule IDs, or drop the branch and expect the raise.

PIC tradeoffs:

• Exit semantics. Exit 2 for any gap-fill incompleteness fails closed but is stricter than core, which exits 2 only on fatal execution failure. Within the same batch run, core-analyzer failures exit 0 (finding 4).
• Surfacing misconfiguration. Removing the broad except Exception exposes real misconfiguration that main hid, such as the pool-mode TypeError. Today the cost is losing that skill's core findings (finding 1).
• Retry cost. Retrying malformed gap-fill output follows core policy and recovers from transient bad output. It also multiplies calls and latency by up to 4 per batch, with no deadline (finding 3).
• BOM-prefixed JSON. A BOM-prefixed but otherwise valid response is now retried and counted as a failed batch, where main dropped it silently. This fails closed, and providers rarely emit a BOM. Stripping a leading U+FEFF in _parse_json_response would accept it instead.
• Empty object. Requiring the findings key (finding 2) means a bare {} is no longer accepted as "nothing found".
• Report shape. Gap-fill ledger rows appear in analysis_completeness only on failure. On success the gap-fill pass is not visible there, so the data shape differs between the two paths.
• Merge order with #796. #796 conflicts textually in contrib/batch_scan/runner.py (the skillspector.llm_analyzer_base import line), and both PRs edit tests/test_batch_scan_security.py. #796 also makes Patch 1 forward timeout, which the deadline fix in finding 3 needs.

Verification and gaps:

• Baseline on main: reproduced with main's own code, as described above. Each failure mode made one call, returned error=None, gap_fill_applied=True, is_complete=True and execution_successful=True, and kept SAFE.
• PR behaviour: checked through my own transcriptions of gap_fill.py:218-306 and runner.py:778-802, run on main's core with and without deepseek_compat.
  • A 200k-deep JSON nesting (RecursionError) gives llm_batch_failed, fatal.
  • Fenced valid JSON succeeds after 1 call.
  • _StructuredResponseValidationError is retried, and provider and runtime errors are not.
  • GapFillAnalyzer overrides parse_response, so compat Patch 2 does not apply to it.
• Completeness recompute: finalize_ledger on real graph results reproduced the graph's own analysis_completeness exactly, on 16 fixtures plus oversized, binary-referenced and >1 MB variants. The PR-style merge adds exactly one gap_fill exception row per failed batch, with no finding_accounting_error or unaccounted_work, including when a successful batch emits a finding.
  • Finding IDs are random (models.py:118-129), so gap findings cannot collide with core IDs.
  • The worker result stays JSON-serializable, because the reason and outcome codes are StrEnum.
• Tests:
  • test_gap_fill_failure_is_incomplete_and_preserves_findings fails on main in every case. So does the new empty-cache assertion at tests/test_batch_scan_security.py:258. Both run in CI through make test-ci.
  • The tests-pro changes do not run in CI.
  • tests/test_batch_scan_security.py passes ruff check and ruff format --check.
• CI: no checks have run. The workflow run on 46af7b5 is action_required (waiting for a maintainer), so the PR description's test counts are unverified, and CI must pass before merge.
• Conflicts: mergeable with main; the main commits merged since the review (#691, #577, #608) touch none of these files. It conflicts with open PR #796 in contrib/batch_scan/runner.py, and both edit tests/test_batch_scan_security.py in different places.
• Head update: I reviewed 46af7b553b1667402d52d8dc2996f806453b6191. The current head a8fb26641167df4b8eadf71213d928e84bc4e23e only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files (#691 only routes structured-output binding through an overridable method in llm_analyzer_base.py), so line references to llm_analyzer_base.py below are to the current head. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Unverified areas:
  • No live provider was available. The recompute was checked against static and no-credential graph results, not against real LLM-analyzer and meta-analyzer ledger rows.
  • The 90 s worker kill in finding 3 is arithmetic from measured call counts and backoff, not an observed kill.
  • Pool mode was reproduced with a stub pool object.
  • Chunked files, where only some chunks of one file fail, were not exercised.
  • Local Python is 3.13; CI uses 3.12.

Decision: Changes Requested (reviewed head 46af7b553b1667402d52d8dc2996f806453b6191; current head a8fb26641167df4b8eadf71213d928e84bc4e23e only adds merges of main)

[SkillSpector Review]

Hi @yashrajp22, thank you for the thorough follow-up. Each of these changes closes an earlier finding directly:

• findings is now a required field.
• Construction and get_batches failures are wrapped in GapFillError, with one failure row per planned file.
• A single per-skill deadline is threaded from run_one through run_gap_fill into the pooled model.

The new setup-failure and real-deadline tests would fail without these changes.

Value and readiness: The gap-fill work is now in good shape. All seven earlier findings are resolved, and I checked each fix by transcribing it onto main's core (details below).

One blocker remains. It comes from the compatibility code this PR copied from #796, not from gap-fill:

• Fixing compat Patch 1 brings the core semantic analyzers back into use in batch mode.
• Compat Patch 2 and Patch 3 are still main's versions, which turn malformed output into an empty, successful result.
• If this PR merges before #796, a statically clean skill whose core LLM reply is malformed or a refusal becomes SAFE. On main the same skill is CAUTION (finding 1).

The fix is either merge order (rebase onto #796 once it lands) or carrying #796's parser changes. Two non-blocking items remain. CI has not run on this head.

Previous findings:

The thread replies cite 45f3024f. That is a merge commit, and it contains the actual fix commits c8d909f9, b758edd3, 3d8ff228 and fe4dc6d3.

1. Resolved. A gap-fill failure before batching replaced the entry with an ERROR stub (formerly runner.py:804, now :824).
   • Fix:
     • run_gap_fill wraps construction and get_batches (gap_fill.py:316-325). It raises GapFillError with one failure row per cache file: runtime_limit for LLMRuntimeLimitError, otherwise llm_batch_failed.
     • _pooled_get_chat_model now accepts timeout (runner.py:90), so the pool-mode TypeError that triggered this finding no longer occurs.
     • Tests at tests/test_batch_scan_security.py:673-702 cover both stages.
   • Evidence (transcription): pool mode on main's real graph, a Chinese skill, every gap-fill response malformed. run_one returned "Gap-fill analysis did not complete for 1 batch(es)." It kept 68/HIGH/DO NOT INSTALL and issues E1/PE3/SC2/TM2, set gap_fill_status=incomplete with reasons ['llm_structured_response_invalid'], and left pool.active_requests=0. On main the new setup-failure test would fail, because run_gap_fill returns [] there and error is None.
   • Author's reply: accurate for setup failures and for batch failures that run_batches_detailed records. A ValueError raised during batch execution still escapes; see finding 2.
2. Resolved. A JSON object without a findings key was accepted as a complete pass (gap_fill.py:267).
   • Fix:
     • GapFillResult.findings is required (gap_fill.py:99).
     • _parse_json_response raises _StructuredResponseValidationError on a JSON or validation error (gap_fill.py:265-269).
     • The CI-run test adds a missing_findings mode.
   • Evidence (transcription on main's core):
     • Each of these took 4 calls and raised GapFillError with ['llm_structured_response_invalid']: {}, {"error": ...}, {"refusal": ...}, a fenced {"error": ...}, {"findings": null}, [] and plain text.
     • Each of these took 1 call and succeeded: {"findings": []}, a BOM-prefixed valid response, a fenced valid response, and {"findings": [], "error": "x"}.
   • Author's reply: accurate.
3. Resolved. Gap-fill had no deadline, so runtime_limit could not occur in production.
   • Fix:
     • run_one computes deadline = start + timeout - min(5, timeout/10) (runner.py:773) and passes a callable timeout to run_gap_fill (:795). That reaches LLMAnalyzerBase through gap_fill.py:205.
     • Patch 1 forwards timeout (runner.py:132-140).
     • batch_scan.py:143 and :210 pass the worker timeout down.
     • PooledChatModel bounds slot waits and key retries by the deadline it receives.
   • Evidence (transcription):
     • Pool mode, L=0.5 s, timeout=6: gap-fill stopped at 5.40 s after 3 calls with ['runtime_limit'], and the core findings were kept.
     • Without the pool, using the real backoff delays: L=0.2 s and timeout=2 gave 2 calls in 1.80 s with runtime_limit.
     • For comparison, the earlier round measured 8 calls in 8.68 s for 2 files at L=0.2 s with no deadline.
   • Author's reply: accurate. The deadline covers gap-fill only; see finding 3, item 2.
4. Resolved. Exit 2 with no machine-readable error.
   • Fix: the author chose option (b).
     • Exit 2 is kept, and enhancements.gap_fill_error, gap_fill_status: "incomplete" and gap_fill_error_reasons are added (runner.py:817-821).
     • 3d8ff228 deliberately leaves entry["error"] unset, so the entry stays in the completeness aggregation (reports.py:53).
     • reports.py:348 copies enhancements into skills[].
     • The exit-code row is updated (contrib/batch_scan/docs/README.md:317).
   • Author's reply: accurate.
5. Resolved. The test did not assert the recommendation or execution_successful.
   • Fix:
     • The test now asserts both (tests/test_batch_scan_security.py:209-210).
     • It adds setup-failure cases (:673-702) and a real-deadline case (:705-737).
     • The synthetic case is relabelled runtime_signal.
   • Evidence:
     • Deleting the SAFE→CAUTION change or the execution_successful copy now fails :209-210.
     • The deadline test fails without the timeout threading (4 calls) and without Patch 1 forwarding timeout (llm_batch_failed instead of runtime_limit).
   • Author's reply: accurate.
6. Resolved. Stale docstrings and docs.
   • Fix:
     • run_gap_fill has a Raises section (gap_fill.py:307-311).
     • The section header is renamed (:273).
     • run_one's Returns section is updated (runner.py:765-770).
     • GapFillError is exported from contrib/batch_scan/__init__.py.
     • The README defines gap_fill_applied (README.md:205-206).
     • The TEST_GUIDE row is updated.
   • Author's reply: accurate. Some new wording nits are in finding 3.
7. Resolved. Duplicated findings comprehension, and a Pydantic branch reachable only from tests.
   • Fix: GapFillError.findings (gap_fill.py:107) is used at runner.py:799. TestParseResponsePydanticModel now asserts ['P5'].
   • Author's reply: accurate.
8. Still open, and now affects behaviour. Merge sequencing with #796.
   • The textual conflict is now mechanical, because #797 copies #796's code verbatim.
   • Merge order now changes behaviour, not just text; see finding 1.

Earlier tradeoff notes:

• BOM-prefixed JSON: now accepted (gap_fill.py:253 strips U+FEFF), in 1 call.
• Bare {}: no longer treated as "nothing found", as intended.
• Report shape: gap-fill ledger rows still appear only on failure. That was a note, not a request.

Material findings

1. [Blocker] contrib/batch_scan/runner.py:140 (Patch 1) together with :147-170 (Patch 2) and :187-215 (Patch 3). Fixing Patch 1 brings the core semantic analyzers back into use in batch mode, but Patch 2 and Patch 3 still turn malformed output into a clean pass.

   • Mechanism:
     • On main, compat Patch 1 (_patched_base_init, main runner.py:127) has no timeout parameter.
     • Since c94b50a (#225) and 73dd1f1 (#393), the core semantic analyzers pass timeout= (for example semantic_security_discovery.py:215).
     • The batch worker and main() always run under deepseek_compat (batch_scan.py:199, :269). So on main, every core semantic analyzer in batch mode fails construction with a TypeError and is reported unavailable.
   • What this PR changes:
     • It copies #796's fixes to Patch 1 (runner.py:132-140) and _pooled_get_chat_model (:90-99), so the core analyzers run again.
     • The copies match #796 exactly, including api_pool.py (blob b068f8b0) and five tests.
     • Patch 2 and Patch 3 are still main's versions. They return [] on JSONDecodeError or a schema error (runner.py:160, :170, :200, :215).
     • With response_schema=None, that [] counts as a successful batch, so the analyzer reports completed.
   • Evidence (transcription): I ran the head's Patch 1 with main's Patch 2 and Patch 3, on main's real graph and main's run_one, under deepseek_compat. On an English skill the PR's run_one skips gap-fill and differs from main's only by the unused deadline.
     • Benign English skill, every core reply not JSON:
       • main: 0 calls, semantic analyzers unavailable, is_complete=False, CAUTION.
       • This PR: 3 calls, all completed, is_complete=True, SAFE.
       • This PR plus #796's Patch 2 (transcribed from #796 runner.py:151-162): 12 calls, degraded, is_complete=False, CAUTION.
     • Schema-invalid reply {"findings":[{"bogus":1}]}: the same split, SAFE under this PR and CAUTION on main.
     • Statically clean English skill with exfiltration intent, where the reply is the plain-text refusal "I cannot help with that request.":
       • This PR: 3 calls, all completed, SAFE, no issues.
       • main: CAUTION.
       • Text in a skill can provoke a refusal, so an attacker can steer this outcome.
     • Malformed meta-analyzer output also reports completed under this PR.
     • Controls:
       • A valid {"findings": []} gives SAFE, which is correct.
       • A valid SSD-1 finding gives DO NOT INSTALL with score 83 under this PR, against CAUTION with score 0 on main.
       • A fixture with static findings is DO NOT INSTALL in all three setups.
   • Context:
     • Patch 2's silent return [] is not new code. It was the live batch behaviour until Patch 1 broke in August.
     • main's CAUTION is a side effect of that breakage, and main also misses every detection that only the LLM analyzers make (see the SSD-1 control).
     • Bringing the analyzers back is a real improvement for well-formed output. The regression is narrower: malformed or refused output on a statically clean skill used to fail closed and now fails open.
     • For non-English skills, a malformed gap-fill reply still gives CAUTION. English skills, which get no gap-fill, are the main exposure.
   • Consequence:
     • If this PR merges before #796, LLM-enabled batch scans report malformed or refused core-analyzer output as a complete, finding-free semantic pass.
     • A statically clean skill is then rated SAFE where main rates it CAUTION. That is the same silent pass this PR removes from gap-fill, reintroduced in the core analyzers.
     • The PR body does not call out the Patch 1 fix or this behaviour change. The live checks in the body ran across the updated PRs together, so they would not catch a standalone merge.
   • Expected fix:

     • Preferred: merge #796 first, rebase this PR onto it, and drop the duplicated hunks:

       • api_pool.py
       • _pooled_get_chat_model
       • Patch 1
       • _patched_chatopenai_init
       • the test_monkeypatch_fragility.py edit
       • the five duplicated tests: test_compat_http_timeout_preserves_shorter_deadline, test_pooled_client_bounds_connect_and_disables_sdk_retries, test_pooled_key_retries_share_one_deadline, test_pooled_request_releases_slot_when_client_build_fails and test_pooled_request_slot_wait_obeys_deadline

       Do not drop Patch 1's timeout forwarding unless #796 is already in. gap_fill.py:205 now passes timeout=, so with main's Patch 1, constructing GapFillAnalyzer would fail. The copy follows from the earlier finding 3, which noted that the deadline needs this Patch 1 change.

     • If this PR must land first: also carry these parts of #796, so that malformed or refused core output under deepseek_compat ends degraded, not SAFE:

       • its Patch 2 and Patch 3 changes (raise _StructuredResponseValidationError on a JSON or schema error)
       • its _verify_patch_targets check for timeout
       • its test_compat_parse_failures_retry_and_remain_visible and test_compat_pooled_constructor_forwards_timeout tests

     • Either way: state in the PR body that core LLM analyzers now run in batch mode.

2. [Non-blocking] contrib/batch_scan/gap_fill.py:326: a ValueError raised during gap-fill batch execution still replaces the entry with an ERROR stub and drops the core findings.

   • Path:

     • Only construction and get_batches are inside the try (gap_fill.py:316-325). run_batches_detailed at :326 is outside it.
     • Core re-raises ValueError from a batch (llm_analyzer_base.py:1373-1374, unchanged).
     • runner.py:797 catches only GapFillError, so the exception reaches runner.py:824-825. There entry_from_error returns issues=[], score 0 and ERROR.
     • (NotImplementedError is effectively unreachable here, because GapFillAnalyzer overrides parse_response.)

   • Triggers:

     • The callable timeout from runner.py:795 now enables the per-call model refresh in _model_for_call for gap-fill, and a ValueError raised there escapes.
     • More broadly, langchain_openai raises ValueError when an OpenAI-compatible endpoint returns HTTP 200 with an error body (langchain_openai/chat_models/base.py:1770-1771). I reproduced this through core run_batches_detailed with a real ChatOpenAI over a mock transport, and it was re-raised after 1 call. So this is not limited to configuration errors.

   • Transcription: I made get_chat_model raise ValueError on the refresh.

     • With gap-fill: error "synthetic: credentials unavailable on refresh", risk {0, ERROR, ERROR}, issues [].
     • The same skill without gap-fill keeps 68/HIGH and issues E1/PE3/SC2/TM2.

   • Consequence:

     • This is not a regression against main, which re-raised ValueError at every stage, and it fails closed.
     • But it contradicts contrib/batch_scan/docs/README.md:439-440 ("Failed setup and failed batches retain core findings") and the new Raises text at gap_fill.py:310 ("Any setup or batch failure").
     • The same ValueError is also handled differently depending on when it happens: the core findings are kept if it is raised at construction (:317), but replaced by an ERROR stub if it is raised during batching.

   • Expected fix: when run_batches_detailed raises, its partial BatchExecutionResult is discarded. A plain try around :326 therefore cannot tell which batches completed. Either:

     • run the batches one at a time in run_gap_fill (run_batches_detailed([batch]) in a loop), and convert a ValueError into a BatchFailure with llm_batch_failed for that batch and the remaining ones; or
     • mark every planned file as failed, and accept losing completed gap-fill findings in that case.

     Add a test in which the model's invoke (or get_chat_model on its second call) raises ValueError. It should assert that the core issue IDs survive and that gap_fill_status == "incomplete". If you prefer to keep these errors hard, narrow the README and docstring wording instead.

3. [Non-blocking] contrib/batch_scan/docs/README.md:437: wording in the new deadline and pool text.

   1. Placement: the new paragraph (README.md:437-443) is appended after the "Next:" navigation footer at :434. The exit-code section (:311-317) is the natural home for it.
   2. Worker-limit sentence: :443 says the worker hard limit applies "if a provider ignores its request timeout". That is accurate for gap-fill, but incomplete for the worker as a whole.
      • The deadline at runner.py:773 reaches only run_gap_fill (:795), and graph.invoke (:777) is not bounded by it.
      • In a measurement on main's code, the core analyzers received a timeout of about 600 s (the shared scan budget).
      • The core analysis runs several LLM stages in sequence, each with up to 4 structured attempts plus pool slot waits. So it can plausibly reach the 90 s kill even when providers behave (not observed).
   3. PR body: "Gap-fill and the provider pool use the remaining per-skill deadline" holds only for gap-fill's pooled calls. PooledChatModel takes the caller's timeout (runner.py:90-99), and the core analyzers pass about 600 s.
   4. Docstrings: the run_one (runner.py:745-771) and run_gap_fill (gap_fill.py:285-312) docstrings do not document the new timeout parameter. (api_pool was already undocumented in run_gap_fill on main.)
   5. Test counts: contrib/batch_scan/tests/docs/TEST_GUIDE.md:42 and :117 still say "41 tests", and the head has 29. The totals were already stale on main, which has 35, but this PR edited the same table at :122 without updating them.
   • Expected fix:
     • Move the paragraph into the exit-code section.
     • Say that the deadline covers gap-fill only, and that core analysis can still reach the worker limit.
     • Correct the pool sentence in the PR body.
     • Document timeout in both docstrings.
     • Fix the TEST_GUIDE totals.

PIC tradeoffs:

• Core analyzers running again in batch mode.
  • Fixing Patch 1 makes LLM-only detections work in batch mode again: score 0 to 83 for a valid SSD-1 reply.
  • The cost is exposing Patch 2 and Patch 3's silent parsing (finding 1), and adding core LLM time to the 90 s worker window.
  • That second cost fails closed (ERROR stub, exit 2), applies equally to #796, and is not fixed by merge order. A real fix would bound core LLM work in batch mode by run_one's deadline, which is outside this PR's scope.
• Exit 2 for any incomplete gap-fill (option b).
  • This fails closed and is now machine-readable.
  • The terminal Errors: count (reports.py:118) still counts only entry["error"], which the README explains.
  • Core-analyzer incompleteness in the same run still exits 0.
• Retries bounded by the deadline.
  • Each batch gets up to 4 attempts within the per-skill budget.
  • A slow provider that keeps returning malformed output now ends as runtime_limit with the findings kept, instead of a worker kill.
• Setup failure marks every cache file as failed. This is conservative but accurate: no gap-fill work happened.
• Copying #796's code. Copying it made the textual conflict mechanical, but it made merge order affect behaviour.

Verification and gaps:

• Reviewed head: 6fdb5733710554616edb3fa54fe69e657d5aa83c. I confirmed with gh pr view that it is still the live head.

• CI: the "CI" workflow run on this head is action_required (waiting for maintainer approval), and there are 0 check runs. The PR body's test counts (542/571, 29 plus 10 subtests, 6,243) and its live NVIDIA Build checks are unverified.

• Conflicts:

  • main (5edc347): merges cleanly.
  • #796 (581ded0): conflicts in contrib/batch_scan/runner.py (the skillspector.llm_analyzer_base import line), contrib/batch_scan/tests/test_monkeypatch_fragility.py and tests/test_batch_scan_security.py. The merged tree has no duplicate test definitions, so the resolution is mechanical.
  • #798 (102bec1): conflicts only in tests/test_batch_scan_security.py, where both PRs append at the end of the file, so the resolution is a union. #798's conflict with main in src/skillspector/cli.py is unrelated to this PR.
  • #741, #793, #794 and #795: no textual conflicts.

• Method: all of this PR's behaviour above comes from my own transcriptions of the head's code, run on main 5edc347:

  • gap_fill.py:96-329, runner.py:90-140 and :772-825, and api_pool.py:478-546
  • #796's Patch 2

  Nothing was imported from the PR.

• Also checked:

  • PooledChatModel now releases its slot when building the client fails, or on CancelledError (api_pool.py:509, :541); on main the slot leaked.
  • reports.py:348 copies the new enhancement fields into skills[], and they are JSON-serializable.
  • Every author commit has a Signed-off-by line. The merges without one are update-branch merges, which ci.yml exempts.
  • tests/test_batch_scan_security.py passes ruff check and ruff format --check.
  • asyncio_mode = "auto" (pyproject.toml:118), so the async pool tests run in CI.

• Gaps:

  • PooledChatModel was exercised with a fake _build_llm. I did not measure real httpx timeout enforcement, the multiprocessing manager RPC path, or whether slot polling is starved under contention; that is also #796's code.
  • That core analysis can reach the 90 s kill comes from code reading plus measured timeout values, not from an observed kill.
  • Under the head's Patch 1 in raw mode, mcp_tool_poisoning raised NotImplementedError ("TP4 requires a structured assessment response"). It was also failed on main, for a different reason. I did not investigate it.
  • BOM acceptance is tested only in tests-pro, which CI does not collect.
  • Local Python is 3.13; CI uses 3.12.

• I did not run the PR's tests or code, per policy.

Decision: Changes Requested (reviewed head 6fdb5733710554616edb3fa54fe69e657d5aa83c)

[SkillSpector Review]

@yashrajp22 thank you for the gap-fill work, and for waiting while #796 landed first.

Value and readiness

This is a status re-review, and the PR's own diff has not changed since the 2026-10-12 review.

• #796 is not in this head. #796 merged to main at 08:16 UTC on 2026-10-13, but this head (0452124b) does not contain it. The last automated merge of main ran at 06:15 UTC, and later ones stopped because the branch now conflicts with main.
• The blocker is therefore still present at this head. Merge order can no longer trigger it, though, and a correct merge of main removes it.
• I simulated that merge, and I expect no blockers once it is pushed.

Material findings

Prior findings (2026-10-12 review):

1. Blocker: still open at this head, and resolved by merging main. contrib/batch_scan/runner.py:140 (open thread): the head still pairs the fixed Patch 1 with the old Patch 2 and Patch 3, which silently return [] (runner.py:160, :170, :200, :215).
   • The simulated merge (git merge-tree) has exactly three conflicts:
     • the llm_analyzer_base import at runner.py:41-42;
     • one test edit in test_monkeypatch_fragility.py;
     • two append regions in tests/test_batch_scan_security.py.
   • Everything else auto-merges to #796's versions: Patch 2 and Patch 3, which raise _StructuredResponseValidationError (main runner.py:151-161 and :181-195), the timeout guard, Patch 1, the pooled factory and api_pool.py.
   • After resolution: the tree differs from main only in the gap-fill changes, and the duplicated hunks and tests drop out.
   • Required: merge main and resolve the conflicts as follows:
     • runner.py: keep both import lines. Do not take this branch's whole side of the file, because that brings back the silent Patch 2 and Patch 3.
     • test_monkeypatch_fragility.py: take main's side, which supersedes this edit.
     • tests/test_batch_scan_security.py: keep both blocks. No test name ends up duplicated.
     • Check: git diff origin/main -- contrib/batch_scan/api_pool.py should then be empty.
2. Non-blocking, still open. gap_fill.py:326 (open thread): a ValueError raised during batch execution still drops the core findings.
3. Non-blocking, still open. contrib/batch_scan/docs/README.md:437 (open thread): the deadline and pool wording, the docstrings and the TEST_GUIDE totals.

Earlier findings 1-7 remain resolved. The old merge-sequencing note is superseded by the conflict described above. The request to state in the PR body that the core analyzers now run in batch mode no longer applies, because #796 made that change on main.

PIC tradeoffs

None new.

Verification and gaps

• git merge-base --is-ancestor 7ee43713 0452124b fails, which confirms that #796 is not in this head.
• I read the conflicted files in the merge-tree result, and diffed that tree against main 8b993d32.
• Since this PR's base, #796 is the only main change to contrib/batch_scan.
• The author has not replied or pushed since the last review. CI is action_required, and it must run on the post-merge head.
• This was a static review. I will re-review the post-merge head.

Decision: Changes Requested (reviewed head 0452124b610e0bcc61ed64ac3da42c18ff602e4b)